Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #1823 - task lists are rendered even when GFM is disabled #1825

Merged
merged 2 commits into from
Nov 19, 2020

Conversation

TheCloudlessSky
Copy link
Contributor

Marked version:

1.2.3

Markdown flavor:

All except GitHub Flavored Markdown

Description

Contributor

  • Test(s) exist to ensure functionality and minimize regression (if no tests added, list tests covering this PR); or,
  • no tests required for this PR.
  • If submitting new feature, it has been documented in the appropriate places.

Committer

In most cases, this should be a different person than the contributor.

@vercel
Copy link

vercel bot commented Nov 15, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/markedjs/markedjs/j6p2afjvu
✅ Preview: https://markedjs-git-tasklist-bug.markedjs.vercel.app

@TheCloudlessSky TheCloudlessSky changed the title Fix #1823 - task lists are rendered even when GFM is disabled. Fix #1823 - task lists are rendered even when GFM is disabled.= Nov 15, 2020
@TheCloudlessSky TheCloudlessSky changed the title Fix #1823 - task lists are rendered even when GFM is disabled.= Fix #1823 - task lists are rendered even when GFM is disabled Nov 15, 2020
Copy link
Member

@UziTech UziTech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 💯

Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

However, this bug has existed for at least a year so we might need to consider it a breaking change for users that have come to rely on this behavior.

@TheCloudlessSky
Copy link
Contributor Author

However, this bug has existed for at least a year so we might need to consider it a breaking change for users that have come to rely on this behavior.

Agree. Is marked ever going to use semantic versioning? Semi-related: I recently upgraded an old version of Marked and it broke even the simplest of Markdown that was supported a while ago with GFM: **Hello ** World would render <strong>Hello </strong> World and now it renders literally **Hello ** World. I found the small breaking-change comments in the release notes was not a strong/obvious enough signal that things really broke vs a major version bump.

@styfle
Copy link
Member

styfle commented Nov 17, 2020

Is marked ever going to use semantic versioning

It already does follow semver, we just have to agree on what is considered a patched bug fix or is breaking.

Nearly every change to marked is going to affect the resulting output so we could consider every change as major semver (besides something like an internal refactor).

However, since we document that we follow the Commonmark/GFM spec, then any change to align with that spec can be considered a patch even though the output changed.

In this case, since its not a fix to the spec but actually a change to the behavior to the public API, I consider this a major semver change.

@UziTech
Copy link
Member

UziTech commented Nov 17, 2020

However, this bug has existed for at least a year so we might need to consider it a breaking change for users that have come to rely on this behavior.

I disagree. First since gfm is true by default most users won't be affected, and as @TheCloudlessSky pointed out we would have to change the major version on every fix just incase someone is relying on it.

xkcd workflow

This is a FIX and should be a patch.

@UziTech
Copy link
Member

UziTech commented Nov 17, 2020

since its not a fix to the spec

This is a fix to the spec.

Before this fix the following markdown did not match CommonMark:

Marked demo

CommonMark demo

- [ ] A
- [ ] B
- [ ] C

@calculuschild
Copy link
Contributor

calculuschild commented Nov 17, 2020

I recently upgraded an old version of Marked and it broke even the simplest of Markdown that was supported a while ago with GFM

For what it's worth, this was another fix that was made to align more closely to the Markdown spec. The extra spaces in **hello ** are invalid markdown and should be rendered as asterisks instead of <strong>. So it's not so much that Marked "dropped support" for a feature, but rather patched out a bug that was incorrectly generating HTML elements that it shouldn't.

It is true that the presence of bugs sometimes means people come to expect a certain behavior, but as @UziTech says, I don't think a patch to fix those bugs requires a major version bump, even if people have unknowingly come to rely on them.

@UziTech
Copy link
Member

UziTech commented Nov 18, 2020

#1834 was just merged to clarify our versioning strategy. https://marked.js.org/publishing#versioning

I think since this PR moves Marked closer to CommonMark compliance and does not touch our public API it should be a patch update.

@UziTech
Copy link
Member

UziTech commented Nov 19, 2020

If this can get one more approval we can merge it

/cc @joshbruce @calculuschild @styfle @davisjam

@UziTech UziTech merged commit 3942e89 into markedjs:master Nov 19, 2020
github-actions bot pushed a commit that referenced this pull request Nov 19, 2020
## [1.2.5](v1.2.4...v1.2.5) (2020-11-19)

### Bug Fixes

* fix em and strong starting with special char ([#1832](#1832)) ([f9bc93b](f9bc93b))
* task lists not rendered when GFM is disabled ([#1825](#1825)) ([3942e89](3942e89)), closes [#1823](#1823)
@github-actions
Copy link

🎉 This PR is included in version 1.2.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Task lists are rendered even when GFM is disabled
4 participants